-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Outputs for provider authors #155
base: main
Are you sure you want to change the base?
Conversation
They are not needed, and they increase implementation complexity.
As of ff4f178, assets, archives and resource references are not supported by the new mapper. We will need to add support for all of these to avoid a regression on merge. |
ff4f178
to
c3c09e4
Compare
Per earlier conversation, my preferred end state here would be to use the same Go SDK for provider authoring as the program authoring. This may mean waiting on Generics, or using pre-Generic pulumi.Output forms from the current Go SDK. If we are indeed headed in that direction, introducing new forms here we'll need to retract them and refactor to the desired end-state some time afterward. This is a little unfortunate. So the question becomes in light of this direction, is this still a change worth taking now? What are the intended use cases or provider we'd like to deploy this at, that motivates doing it now vs later, would that be something that can help with Docker Image perhaps? How about external ecosystem providers depending on pulumi-go-provider do we have a sense of how many there are and how much nuisance we're imposing by introducing these types first and then asking them to take a breaking change? Is the ecosystem /uptake small enough this is not a worry at the moment? |
My concern here is that our I think more work is needed to check if we can unify these definitions. If
I view this change (or one like it) as necessary for shipping a high quality docker provider (that handles unknowns well). I'll defer to the @blampe on if he is interested in adopting the provider framework (with or without this change) for Docker. This change is opt-in and non-breaking for existing providers, so I'm not concerned about a break during adoption. I think that the ecosystem that is using this framework is willing to take breaking changes to be able to write better providers. The README.md still says:
|
Can you elaborate that please? AFAIK, every Output in a pulumi Go program is force-awaited before the program exists, so if you need Are there any other semantic differences? BTW we do not need to wait for generic pulumi.Output[T] to land afaik as we can start using non-generic pulumi.Output perhaps as of today. |
https://github.com/pulumi/pulumi/blob/master/sdk/go/internal/types.go#L380 and WorkGroup.Wait() for reference. Also https://github.com/pulumi/pulumi/blob/master/sdk/go/internal/state.go#L44 |
I think Go is the one language where it is guaranteed that Outputs will resolve, because there's no lifting from promises. |
I'm definitely interested in anything we can do to make provider development easier and safer! I'll try to grab some time with you to learn more about how this could help docker.
I'm still gathering context, but if |
I need the user to have a
Field dependency is injected during deserialization and then merged during applies. It is then rehydrated during serialization.
Assuming we taught type Input struct {
Field infer.Output[Nested] `pulumi:"nested"`
}
type Nested struct {
N1 infer.Output[string] `pulumi:"n1"`
N2 infer.Output[int] `pulumi:"n2"`
} Expressing nested values in
Is that exposed anywhere? I would love to use
|
That's not safe. Providers can get called with unknown values where there is no T. Further you lose all secret and dependency tracking when you try to exit the context like that. The Go SDK has an UnsafeAwait for Output that returns a struct of Value, Known, Secret, Deps. But working with that is a pain, and I doubt anyone uses it fully correctly. My opinion is any design where you try to exit the Output context is wrong. If there's something unexpressible with Apply we should fix Apply. |
Looks like this is still in my inbox, stalling a bit. Throw a meeting to unblock? Or icebox for now?
I believe what's implied here is UnsafeAwaitOutput from the Go SDK pulumi/pulumi#10877 I'd think it is fair game for the provider implementation framework to call that one when needed, which is not the same thing as making it necessary to call for provider authors using the framework. Does this help untie the knot here @iwahbe ? |
@blampe I believe you are the current expert consumer of this library. Do you believe that this is worth pushing through, or should we put on hold.
I still think that unless In Pulumi program land, an In pulumi-go-provider land, an
For each In addition, |
This are not very convincing arguments to me, as forking the foundational types is still a high cost.
|
Inclined to agree with @t0yv0 here. Outputs in Pulumi programs will resolve unless users have done very bad things (infinite loops in apply for example). You say |
This is not true during preview.
I'm saying that I need to query if the output is resolvable or not, which is different than resolved. I don't know if
We do not use To be able to use
v := unstable.NewOutputWithPayload[String, infer.dependency](unstable.RawOutput{
Value: resource.PropertyOf("value"),
Secret: true
Computed: false,
}, infer.dependency{ fields: "valueField" })
// Give to user
i := pulumi.Apply1E(v, strconv.ParseInt)
return Args{I: I}
// Back in framework space
deps := unstable.OutputPayload[infer.dependency](result.I) // infer.dependency{ fields: "valueField" }
We need the following logic to be expressible:
I actually think that (1) and (2) are solvable if // unstable/output.go -- somewhere in pulumi/pulumi
func NewOutputWithPayload[T Value, P any, PG PayloadManager[P]](output RawOutput, payload P, payload) { ... }
func GetPayload[P Payload](o internal.Output) (P, bool) { ... }
type PayloadManager[P] interface {
func Merge(ctx context.Context, outputs []struct{p P, o RawOutput}) P
func New(ctx context.Context, output RawOutput) P
}
type RawOutput = internals.UnsafeAwaitOutputResult Maybe I'm misunderstanding something and this is already possible. |
Can you give an example of an output that's not resolvable? Everything passed to the provider is resolvable. In preview it may resolve to an unknown. On top of this you have provider implementation that constructs resources and generates derived Outputs. I think in provider such as TF providers all I can think of is that all such outputs will be resolvable as well. They may resolve to the unknown value. What's an unresolvable output? That would help me understand. |
We might be talking past each-other. I think of this as unresolvable:
This is unresolved, but resolvable:
If we allow something to resolve to unknown, then I agree that all values are resolvable. |
In Go result, err := UnsafeAwaitOutput(ctx, UnknownValue)
require.NoError(t, err)
require.False(t, result.Known) |
Another tangent, based on your explanation of what you mean by "will resolve", this logic seems to substitute zero values for unknowns, which is a massive footgun IMHO, admittedly one that is present in our prior designs. If we're designing from first principles, why is this ever needed to inject zero values?
Could we force the provider author to model locations that might not be known with a type that explicitly models unknowns, e.g. an Input or an Output? |
We do allow that. As @t0yv0 said all SDKs will resolve I massively agree with Anton about being very careful about zero values around this.
It might be this is at the point where we really ought to have a meeting about it. Provider/Core ideation is coming up, maybe enough to just be a point in that. |
Maybe it would help to clarify the primary problem this is aimed at? I see a few potential benefits:
(1) seems like a good thing, but it also seems like we could accomplish this with existing types. For example we should be able to give the user the option to hydrate secrets as type Foo struct {
RawSecret string `pulumi:"raw,optional" provider:"secret"`
WrappedSecret pulumix.Output[string] `pulumi:"wrapped,optional" provider:"secret"`
} I might go further and say this deserves its own (2) I haven't run into yet, although I also haven't been focused on preview behavior. I do anticipate some gotchas around validation, and my plan is to basically special case zero-values during previews. I do think it would be helpful to allow a user to determine if a value is unknown, although I don't think we need all the type Foo struct {
OptionalRaw string `pulumi:"raw,optional"`
Optional Optional[string] `pulumi:"opt,optional"`
OptionalSecret Optional[Secret[string]] `pulumi:"secret,optional" provider:"secret"`
} I realize you can accomplish essentially the same thing with (3) is a non-goal IMO. As a user I would much rather the harness handle resolving everything before invoking my code versus me needing to know or care about unresolved things (the exception being (2) above). If we do really need a micro-optimizations like this (I'm skeptical), I would prefer that to be an alternative code path instead of sacrificing simplicity in the common case. |
:screaming-intensifies: Optional is not the same as zero is not the same as unknown. These are three very distinct values, please for all that's holy can we design a system that understands that. I get that we're working with Go and it's a hot mess for specifying anything but I'd hope we can do better than just sticking with Go's (frankly stupid) opinion that zero values are always good sensible defaults and can just be used for everything.
Inputs to providers are either plain (not secret, not unknown) or output'y (potentially unknown, potentially secret). I don't think there's any cases where we allow secret but don't allow unknown.
We really need to define what we mean by resolved. As far as I know all inputs to providers are always fully resolved. The engine does not send async values to providers, it waits for all inputs to resolve and then sends those resolved values. Even Call and Construct wait for resolution. But when I say resolved I mean it has a value, but that value might be "unknown". I suspect most of you are using resolved for "not unknown"? Either way, there's no value for callbacks in providers. The values either known or not and you know that as soon as you're called and it's not going to asynchronously change. |
Haha, I start observing words like holy and since religious feelings are involved we should definitely meet before writing any more :) (oh I feel those strong feeling also). |
BTW I fully expect this design space is a solution that satisfies all the comments.. And Bryce's and Fraser's comments both have merit. And Ian had some more information out of band. Take it easy folks we'll sort this one out! |
The goal is (1) and (2) with I have currently implemented I think that we have spent enough time here that this is worth a synchronous meeting. I'll write up a doc and present during IaC ideation.
This does convince me that a change like this is well motivated. |
This PR provides a new type available to
infer
authors:infer.Output[T]
. This type can be used for any field withpulumi:"{name}"
in place ofT
. It provides secret + unknown metadata and an unknown safeinfer.Apply
API modeled afterpulumix
as implemented in the bridge.The
file
example has been converted to use the new API.Implementors Guide
We need a generic type (
Output[T]
) that has a couple of properties:resource.PropertyValue
andOutput[T]
(when types align). This means thatOutput[T]
needs to understand secrets and unknown values.Output[T]
.pulumix
, with easy access to escape.Implementation follows the needs of the new feature closely. There are two major areas of change:
resource.PropertyValue
toT
for anyT
. This new mapper will preserve information whenT
isOutput[U]
. This mapper is part of the internalende
package, butende
could conceivably be moved outside of theinfer
package entirely.infer.Output[T]
type that fulfills the same basic contract aspulumix.Output[T]
.infer.Output[T]
is different in several ways:ende
.Output[T].Anchor() error
blocks the thread until the output has resolved, if it can.Output[T].GetKnown() (T, error)
retrieves the known value (or an error in the computation chain). It panics if the value is unknowable.Output[T].GetMaybeUnknown() (T, error)
retrieves the known value or zero value (or an error in the computation chain).